Skip to content

[compiler] Improve impurity/ref validation#225

Open
everettbu wants to merge 5 commits intomainfrom
pr35298
Open

[compiler] Improve impurity/ref validation#225
everettbu wants to merge 5 commits intomainfrom
pr35298

Conversation

@everettbu
Copy link
Copy Markdown

@everettbu everettbu commented Dec 13, 2025

Mirror of facebook/react#35298
Original author: josephsavona


Summary

note: This implements the idea discussed in https://github.com/reactwg/react/discussions/389#discussioncomment-14252280

This is a large PR that significantly changes our impurity and ref validation to address multiple issues. The goal is to reduce false positives and make the errors we do report more actionable.

Validating Against Impure Values In Render

Currently we create Impure effects for impure functions like Date.now() or Math.random(), and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok).

This PR updates to validate that impure values are not transitively consumed during render, building on the new effects system: rather than look at instruction types, we use effects like Capture a -> b, Impure a, and Render b to determine how impure values are introduced and where they flow through the program. We're intentionally conservative, and do not propagate impurity through MaybeCapture effects, which is used for functions we don't have signatures for. This means that things like identity(performance.now()) drop the impurity. This feels like a good compromise since it means we have very high confidence in the errors that we report and can always add increased strictness later as our confidence increases.

An example error:

Error: Cannot access impure value during render

Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent).

error.invalid-impure-value-in-render-helper.ts:5:17
  3 |   const now = () => Date.now();
  4 |   const render = () => {
> 5 |     return <div>{now()}</div>;
    |                  ^^^^^ Cannot access impure value during render
  6 |   };
  7 |   return <div>{render()}</div>;
  8 | }

error.invalid-impure-value-in-render-helper.ts:3:20
  1 | // `@validateNoImpureFunctionsInRender`
  2 | function Component() {
> 3 |   const now = () => Date.now();
    |                     ^^^^^^^^^^ `Date.now` is an impure function.
  4 |   const render = () => {
  5 |     return <div>{now()}</div>;
  6 |   };

Impure values are allowed to flow into refs, meaning that we now allow useRef(Date.now()) or useRef(localFunctionThatReturnsMathDotRandom()) which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well.

Refs Now Treated As Impure Values in Render

Reading a ref now produces an Impure effect, and reading refs in render is validated using the above validation against impure values in render. The error category and message is customized for refs, we're just reusing the validation implementation. This means you get consistent results for performance.now() as for ref.current. A nice consistency win.

Simplified writing-ref validation

Now that reading a ref in render is validated using the impure values infra, I also dramatically simplified ValidateNoRefAccessInRender to focus solely on validation against writing refs during render. It was harder to use the new effects infra for this since we intentionally do not record ref mutations as a Mutate effect. So for now, the pass switches on InstructionValue variants. We continue to support the if (ref.current == null) { ref.current = <init> } pattern and reasonable variants of it. We're conservative about what we consider to be a write of a ref - foo(ref) now assumes you're not mutating rather than the inverse.

Takeaways

  • Impure-values-in-render logic is more conservative about what it considers an error (ie to users it appears more persmissive). We follow clearly identifiable (and if we wanted traceable/explainable) paths from impure sources through to where they are rendered. We allow many more cases than before, notably x = foo(ref) optimistically assumes you don't read the ref. Concretely, this follows from not tracking impure values across MaybeCapture effects.
  • No-writing-refs-in-render is also more conservative about what it counts as a write, appearing to users as allowing more cases. We look for very direct evidence of writing to refs, ie ref.current = <value> or ref.current.property = <value>, and allow potential writes through eg writeToRef(ref, value).

Stack created with Sapling. Best reviewed with ReviewStack.

  • #35607
  • -> #35298
  • #35596
  • #35573
  • #35595
  • #35539

@everettbu everettbu added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 13, 2025
@everettbu everettbu force-pushed the pr35298 branch 5 times, most recently from 9899f74 to d4c7683 Compare January 6, 2026 02:52
@everettbu everettbu changed the title [compiler][poc] Improve impurity/ref tracking [compiler] Improve impurity/ref tracking Jan 6, 2026
@everettbu everettbu marked this pull request as ready for review January 6, 2026 02:54
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jan 6, 2026

Greptile Summary

This PR significantly refactors the React compiler's impurity and ref validation system to reduce false positives and improve error actionability.

Key Changes:

  • Replaces instruction-type-based impurity checking with effects-based tracking that follows values through the program
  • Impure values (from Date.now(), Math.random(), etc.) are now tracked through Impure effects and only error if they reach a Render effect
  • Refs are treated as impure values - reading ref.current now produces an Impure effect validated the same way
  • Validation is intentionally conservative: impurity is not propagated through MaybeCapture effects (functions without signatures), so identity(Date.now()) drops the impurity
  • Ref mutation validation simplified to focus only on writes, allowing patterns like foo(ref) which previously errored
  • The if (ref.current == null) { ref.current = value } null-guard initialization pattern continues to be allowed

New Behavior:

  • Now allowed: useRef(Date.now()), useRef(localFunctionThatReturnsMathRandom()), passing refs to functions, reading refs in non-rendered contexts like console.log(ref.current)
  • Still errors: Direct rendering of impure values, reading refs in rendered output, writing refs during render (except in null-guards)

Architecture:
The new ValidateNoImpureValuesInRender.ts pass uses a fixed-point algorithm to propagate impurity through the HIR, building signatures for nested functions. The Impure effect type was enhanced with detailed error messaging fields for better diagnostics.

Confidence Score: 4/5

  • This PR is generally safe to merge with minor documentation issues that should be addressed
  • The core logic changes are well-structured and improve validation accuracy by using the effects system. However, there are a few test fixtures with typos in fixture data and misleading comments that could confuse future maintainers
  • Pay close attention to test fixture files with typos and misleading comments that should be corrected

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureValuesInRender.ts New validation pass that tracks impure values through effects system instead of instruction types, following Impure effects from sources through Capture/Assign to Render sites
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts Simplified to focus solely on ref mutations during render, now conservative about what counts as writes (only direct ref.current = value patterns)
compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts Enhanced Impure effect type with detailed error messaging fields (category, reason, description, usageMessage, sourceMessage) for better diagnostics
compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts Updated to create Impure effects for ref property loads and impure function calls, with detailed error messages embedded in effects
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/validate-mutate-ref-arg-in-render.expect.md Test fixture with typo in fixture data (cuurrent instead of current), but correctly tests that console.log(ref.current) is now allowed
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/error.invalid-impure-functions-in-render-indirect-via-mutation.expect.md Test with misleading comment claiming behavior is "Allowed" but correctly expects error - impurity is tracked through arrayPush mutation

@everettbu everettbu force-pushed the pr35298 branch 3 times, most recently from 28f77b4 to b5ec0ca Compare January 16, 2026 17:11
@everettbu everettbu changed the title [compiler] Improve impurity/ref tracking [compiler] Improve impurity/ref validation Jan 16, 2026
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (2)

  1. compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/validate-mutate-ref-arg-in-render.js, line 11 (link)

    syntax: Typo in 'cuurrent' should be 'current'

  2. compiler/packages/babel-plugin-react-compiler/src/HIR/TypeSchema.ts, line 240 (link)

    syntax: Missing RenderEffectSchema in the union - should be included after ApplyEffectSchema

111 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile


export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{bar: 'foo'}, {ref: {cuurrent: 1}}],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Typo in property name: 'cuurrent' should be 'current'

Suggested change
params: [{bar: 'foo'}, {ref: {cuurrent: 1}}],
params: [{bar: 'foo'}, {ref: {current: 1}}],
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/validate-mutate-ref-arg-in-render.expect.md
Line: 15:15

Comment:
**syntax:** Typo in property name: 'cuurrent' should be 'current'

```suggestion
  params: [{bar: 'foo'}, {ref: {current: 1}}],
```

How can I resolve this? If you propose a fix, please make it concise.


export const FIXTURE_ENTRYPOINT = {
fn: Foo,
params: [{ bar: "foo" }, { ref: { cuurrent: 1 } }],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Same typo propagated to compiled output: 'cuurrent' should be 'current'

Suggested change
params: [{ bar: "foo" }, { ref: { cuurrent: 1 } }],
params: [{ bar: "foo" }, { ref: { current: 1 } }],
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/validate-mutate-ref-arg-in-render.expect.md
Line: 43:43

Comment:
**syntax:** Same typo propagated to compiled output: 'cuurrent' should be 'current'

```suggestion
  params: [{ bar: "foo" }, { ref: { current: 1 } }],
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +10 to +12
* Allowed: we don't have sufficient type information to be sure that
* this accesses an impure value during render. The impurity is lost
* when passed through external function calls.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Comment contradicts test expectation - claims this should be "Allowed" but test expects an error

Suggested change
* Allowed: we don't have sufficient type information to be sure that
* this accesses an impure value during render. The impurity is lost
* when passed through external function calls.
/**
* Error: impure value accessed during render. Even though the impure value
* is passed through external function calls, the compiler can still track
* that it flows to the rendered output.
*/

Should this test case actually pass or is the comment outdated?

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect-via-mutation.expect.md
Line: 10:12

Comment:
**logic:** Comment contradicts test expectation - claims this should be "Allowed" but test expects an error

```suggestion
 /**
 * Error: impure value accessed during render. Even though the impure value
 * is passed through external function calls, the compiler can still track
 * that it flows to the rendered output.
 */
```

 Should this test case actually pass or is the comment outdated?

How can I resolve this? If you propose a fix, please make it concise.

Much nicer workflow for working through errors in the compiler:
* Run `yarn snap -w`, oops there are are errors
* Hit 'p' to select a fixture => the suggestions populate with recent failures, sorted alphabetically. No need to copy/paste the name of the fixture you want to focus on!
* tab/shift-tab to pick one, hit enter to select that one
* ...Focus on fixing that test...
* 'p' to re-enter the picker. Snap tracks the last state of each fixture and continues to show all tests that failed on their last run, so you can easily move on to the next one. The currently selected test is highlighted, making it easy to move to the next one.
* 'a' at any time to run all tests
* 'd' at any time to toggle debug output on/off (while focusing on a single test)
…opment

Autogenerated summaries of each of the compiler passes which allow agents to get the key ideas of a compiler pass, including key input/output invariants, without having to reprocess the file each time. In the subsequent diff this seemed to help.
Optional chaining and other value blocks within try/catch blocks were throwing an Invariant error ("Unexpected terminal in optional") instead of the expected Todo error. This caused hard failures instead of graceful bailouts.

The issue occurred because DropManualMemoization and ValidateExhaustiveDependencies ran before BuildReactiveFunction, and encountered `maybe-throw` terminals in their switch statements without a handler, falling through to the invariant case.

Fixed by adding explicit `maybe-throw` cases in both files that throw the proper Todo error with the message "Support value blocks (conditional, logical, optional chaining, etc) within a try/catch statement".

Also renamed the existing bug test to reflect it's now correctly handled:
- error.bug-invariant-unexpected-terminal-in-optional → error.todo-optional-chaining-within-try-catch

Closes #35570
Fixes a longstanding issue where we didn't support code like `useFoo(value?.bar(), value?.bar()) ?? {}` - we would attempt to construct a ReactiveFunction, recursively processing the blocks, but the inner optional `value?.bar()` wouldn't match with what the outer optional was expecting to find. It's a one-line fix!

Note: memoization in the examples is not ideal, but i've confirmed that it is not strictly related to the optional issue.
# Summary

note: This implements the idea discussed in reactwg/react#389 (comment)

This is a large PR that significantly changes our impurity and ref validation to address multiple issues. The goal is to reduce false positives and make the errors we do report more actionable.

## Validating Against Impure Values In Render

Currently we create `Impure` effects for impure functions like `Date.now()` or `Math.random()`, and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok).

This PR updates to validate that impure values are not transitively consumed during render, building on the new effects system: rather than look at instruction types, we use effects like `Capture a -> b`, `Impure a`, and `Render b` to determine how impure values are introduced and where they flow through the program. We're intentionally conservative, and do _not_ propagate impurity through MaybeCapture effects, which is used for functions we don't have signatures for. This means that things like `identity(performance.now())` drop the impurity. This feels like a good compromise since it means we have very high confidence in the errors that we report and can always add increased strictness later as our confidence increases.

An example error:

```
Error: Cannot access impure value during render

Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent).

error.invalid-impure-value-in-render-helper.ts:5:17
  3 |   const now = () => Date.now();
  4 |   const render = () => {
> 5 |     return <div>{now()}</div>;
    |                  ^^^^^ Cannot access impure value during render
  6 |   };
  7 |   return <div>{render()}</div>;
  8 | }

error.invalid-impure-value-in-render-helper.ts:3:20
  1 | // @validateNoImpureFunctionsInRender
  2 | function Component() {
> 3 |   const now = () => Date.now();
    |                     ^^^^^^^^^^ `Date.now` is an impure function.
  4 |   const render = () => {
  5 |     return <div>{now()}</div>;
  6 |   };
```

Impure values are allowed to flow into refs, meaning that we now allow `useRef(Date.now())` or `useRef(localFunctionThatReturnsMathDotRandom())` which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well.

## Refs Now Treated As Impure Values in Render

Reading a ref now produces an `Impure` effect, and reading refs in render is validated using the above validation against impure values in render. The error category and message is customized for refs, we're just reusing the validation implementation. This means you get consistent results for `performance.now()` as for `ref.current`. A nice consistency win.

## Simplified writing-ref validation

Now that _reading_ a ref in render is validated using the impure values infra, I also dramatically simplified ValidateNoRefAccessInRender to focus solely on validation against _writing_ refs during render. It was harder to use the new effects infra for this since we intentionally do not record ref mutations as a `Mutate` effect. So for now, the pass switches on InstructionValue variants. We continue to support the `if (ref.current == null) { ref.current = <init> }` pattern and reasonable variants of it. We're conservative about what we consider to be a write of a ref - `foo(ref)` now assumes you're not mutating rather than the inverse.

# Takeaways

* Impure-values-in-render logic is more conservative about what it considers an error (ie to users it appears more persmissive). We follow clearly identifiable (and if we wanted traceable/explainable) paths from impure sources through to where they are rendered. We allow many more cases than before, notably `x = foo(ref)` optimistically assumes you don't read the ref. Concretely, this follows from not tracking impure values across MaybeCapture effects.
* No-writing-refs-in-render is also more conservative about what it counts as a write, appearing to users as allowing more cases. We look for very direct evidence of writing to refs, ie `ref.current = <value>` or `ref.current.property = <value>`, and allow potential writes through eg `writeToRef(ref, value)`.
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/validate-mutate-ref-arg-in-render.js, line 10 (link)

    syntax: Typo in fixture data: cuurrent should be current

215 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +5 to +9
/**
* Allowed: we don't have sufficient type information to be sure that
* this accesses an impure value during render. The impurity is lost
* when passed through external function calls.
*/
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Comment is misleading - claims "Allowed" but test expects an error

The comment states this case is allowed because "impurity is lost when passed through external function calls", but the test fixture expects an error. The implementation correctly tracks impurity through the arrayPush mutation and renders it, so it should error. Update comment to match actual behavior.

Suggested change
/**
* Allowed: we don't have sufficient type information to be sure that
* this accesses an impure value during render. The impurity is lost
* when passed through external function calls.
*/
/**
* The impure value flows through arrayPush into the array and is rendered.
* Even though arrayPush is an external function, the impurity is tracked
* through the mutation and reaches the render site.
*/
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-impure-functions-in-render-indirect-via-mutation.js
Line: 5:9

Comment:
**style:** Comment is misleading - claims "Allowed" but test expects an error

The comment states this case is allowed because "impurity is lost when passed through external function calls", but the test fixture expects an error. The implementation correctly tracks impurity through the `arrayPush` mutation and renders it, so it should error. Update comment to match actual behavior.

```suggestion
/**
 * The impure value flows through arrayPush into the array and is rendered.
 * Even though arrayPush is an external function, the impurity is tracked
 * through the mutation and reaches the render site.
 */
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +100 to +113
if (
sourceEffect == null &&
(effect.kind === 'Assign' || effect.kind === 'Capture') &&
!impure.has(effect.from.identifier.id) &&
!rendered.has(effect.from.identifier.id) &&
!isUseRefType(effect.from.identifier) &&
isMutable({id}, effect.into)
) {
const destinationEffect = impure.get(effect.into.identifier.id);
if (destinationEffect != null) {
impure.set(effect.from.identifier.id, destinationEffect);
hasChanges = true;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Bidirectional impurity propagation handles mutations like arrayPush(array, impureValue) - when an impure value is captured into a mutable destination, the destination becomes impure and can propagate forward to render sites

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoImpureValuesInRender.ts
Line: 100:113

Comment:
**style:** Bidirectional impurity propagation handles mutations like `arrayPush(array, impureValue)` - when an impure value is captured into a mutable destination, the destination becomes impure and can propagate forward to render sites

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants